Skip to content
This repository was archived by the owner on Jul 22, 2025. It is now read-only.

Conversation

@martin-brennan
Copy link
Contributor

@martin-brennan martin-brennan commented Nov 26, 2024

This commit applies further admin UI guidelines, now that they have been more
fleshed out in core, to the AI admin UI:

  • Tools
  • LLMs
  • Personas

The changes include but are not limited to:

  • Applying the table CSS classes, for desktop and mobile
  • Adding a description and learn more link for each tab
  • Adding an empty list placeholder with CTA using AdminConfigAreaEmptyList
  • Replacing custom headings with AdminPageSubheader

Desktop

image
image
image

Mobile

image
image
image

@martin-brennan martin-brennan force-pushed the ux/apply-more-admin-ui-guidelines branch from 2c16d21 to 60e83fe Compare November 26, 2024 23:06
@martin-brennan martin-brennan force-pushed the ux/apply-more-admin-ui-guidelines branch from b88b520 to ee15154 Compare November 27, 2024 02:39
@martin-brennan martin-brennan changed the title WIP: Applying more admin UI guidelines UX: Applying more admin UI guidelines Nov 27, 2024
@martin-brennan martin-brennan marked this pull request as ready for review November 27, 2024 02:47
Copy link
Contributor

@tyb-talks tyb-talks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally lgtm, had a question on the tr/td class assertions.

it "shows the provider as CDCK in the UI" do
visit "/admin/plugins/discourse-ai/ai-llms"
expect(page).to have_css(
"[data-llm-id='cdck-hosted'] .column-provider",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed that we are treating the table wrapper classes as extraneous (e.g. admin-row__detail, d-admin-row__controls) during the assertions. Is this to ensure the tests aren't too fragile? Assertions on those would enforce that these elements keep to classes that align with the admin UI guidelines.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I just think they are unnecessary since we are already targeting the row by unique ID. I do agree there should be some way of making sure people use the classes from the UI guidelines, but I'm not sure what just yet...for things like the headings I made reusable components, and we could conceivably do that here too:

<DAdminTable class="ai-tool-list-editor" as |table|>
  <table.Head>
    <th>{{i18n "discourse_ai.tools.name"}}</th>
    <th></th>
  </table.Head>
  <table.Body>
    {{#each @tools as |tool|}}
      <table.Row idAttr="tool-id" id={{tool.id}} as |row|>
        <row.Overview>
          <div class="ai-tool-list__name-with-description">
            <div class="ai-tool-list__name">
              <strong>
                {{tool.name}}
              </strong>
            </div>
            <div class="ai-tool-list__description">
              {{tool.description}}
            </div>
          </div>
        </row.Overview>
        <row.Controls>
          <LinkTo
            @route="adminPlugins.show.discourse-ai-tools.show"
            @model={{tool}}
            class="btn btn-text btn-small"
          >{{I18n.t "discourse_ai.tools.edit"}}</LinkTo>
        </row.Controls>
      </table.Row>
    {{/each}}
  </table.Body>
</DAdminTable>

But this feels like kind of overkill. Not sure of a more lightweight solution for this, but I am thinking about it as we add more and more of these table classes.

@martin-brennan martin-brennan merged commit 2f7895b into main Nov 27, 2024
6 checks passed
@martin-brennan martin-brennan deleted the ux/apply-more-admin-ui-guidelines branch November 27, 2024 03:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants